feat: support mount SMB Azure File with user-provided OAuth token through k8s secret#3100
Conversation
552cc56 to
c2bd547
Compare
4188373 to
18fe595
Compare
18fe595 to
fa39190
Compare
There was a problem hiding this comment.
Pull request overview
Adds support for mounting Azure File shares using a user-provided managed identity OAuth token (via Kubernetes Secret) to enable cross-tenant scenarios, integrating this as a new volume context parameter and wiring it into NodeStageVolume credential-cache setup.
Changes:
- Add
mountwithmanagedidentitytokenvolume context parameter and secret fieldazurestoragemanagedidentitytoken. - Extend credential-cache setup to support passing an OAuth token directly to
azfilesauthmanager set. - Update node staging logic to handle the new MI-token path and enforce mutual exclusivity with existing identity-based options.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/azurefile/utils.go | Extends setCredentialCache to accept either tokenFile-based WI flow or direct token flow; redacts token in logs. |
| pkg/azurefile/utils_test.go | Updates setCredentialCache call signature in tests. |
| pkg/azurefile/nodeserver.go | Adds parsing/validation for new volume context flag and implements setCredentialCacheWithMIToken called during SMB mount. |
| pkg/azurefile/nodeserver_test.go | Updates mutual exclusivity test/error message to include the new flag. |
| pkg/azurefile/azurefile.go | Adds new constants and extends GetStorageAccountFromSecret to return the MI token; recognizes the new mount mode in GetAccountInfo. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
6f727c1 to
89e66ed
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
5c07d5e to
927abaf
Compare
Use status.Errorf instead of fmt.Errorf so the error surfaces as codes.Internal rather than codes.Unknown through status.Convert.
Use status.Errorf instead of fmt.Errorf so the error surfaces as codes.Internal rather than codes.Unknown through status.Convert.
- CreateVolume: validate secretName is provided when mountWithOAuthToken is true, fail early with InvalidArgument instead of deferring failure to NodePublish - e2e: use shared oauthSecretName/oauthSecretNamespace constants instead of hardcoded strings in dynamic provisioning test - utils_test: add test case for mutually exclusive token and tokenFile
- CreateVolume: return InvalidArgument when mountWithOAuthToken is combined with NFS protocol, preventing provisioning shares that will fail at mount time - e2e: add trailing slash to IMDS resource URL (https://storage.azure.com/) to avoid invalid_resource rejection in some environments
NFS can be indicated via either protocol=nfs or fsType=nfs. The mountWithOAuthToken guard only checked protocol, so a PV with fsType: nfs could bypass the check. Reject both.
In CAPZ clusters, the OIDC JWKS document is served from Azure Blob Storage and may not be immediately available after cluster creation. When the CSI driver tries to mount with workload identity, it exchanges the SA token for an Azure OAuth token via AAD. If AAD cannot find the signing key in the OIDC issuer's JWKS metadata, it returns AADSTS7000272 / 400 Bad Request and the mount fails. Changes: - Add 120s wait for FIC and RBAC propagation after setup - Add waitForOIDCJWKS() that polls the issuer's JWKS endpoint until it returns at least one signing key (up to 5 min timeout)
…AuthToken Instead of wrapping the error with codes.Internal, return the error directly so that InvalidArgument and other specific status codes from setCredentialCacheWithOAuthToken are preserved.
…essages - Extract mountWithOAuthToken validation to shared validateMountWithOAuthToken() - Log secret name and namespace when using OAuth token - Improve error message: clientID must be provided when tokenFile is set
|
/test |
|
/retest |
1 similar comment
|
/retest |
|
The secret reference comes from the PV/StorageClass which is provisioned by a cluster admin, but the intention is that the OAuth token represents the user's identity. There's no per-pod scoping and any pod that can mount the PV gets the secret's privileges. Worth a note in the docs that the secret must be treated as cluster-scoped credentials, and secretNamespace should be a namespace not writable by tenants. |
added |
| setKeyValueInMap(context, secretNamespaceField, context[podNamespaceField]) | ||
| // When Managed Identity is used for ephemeral volumes then reject the request. | ||
| // Allowing access for inline volume with identity will open up risk of arbitrary pods accessing fileshares with node identity permissions. | ||
| if strings.EqualFold(getValueInMap(context, mountWithManagedIdentityField), trueValue) { |
There was a problem hiding this comment.
Could we block the OAuth here for inline volumes explicitly like it is done for the managed identity?
|
Since dynamic provisioning is supported, I think we should force the secret namespace to match pod's namespace. Because a user that doesn't have access to view/list secrets in another namespace might be able to exploit the dynamic provisioning path and have csi driver lookup and use the secret with driver's rbac. Or we can also block the dynamic provisioning path for this auth option. |
Explicitly reject OAuth token authentication for inline (ephemeral) volumes, similar to the existing managed identity check. Inline volumes should not use identity-based auth as it would allow arbitrary pods to access fileshares with the provided token's privileges.
@landreasyan in dynamic provisioning, we don't do such secret namespace enforcement since storage class is not namespaced, and admin configures storage class. We only force the secret namespace to match pod's namespace in inline volume since pod is namespaced, and pod is managed by normal user. It behaves same as when we use storage account key mount, configure a different secret namespace in CSI driver storage class should be supported. |
We don't need the inline or dynamic support for the issue that was reported. Can we block these scenarios in this PR and merge them later when the security aspects are further discussed? These would be hard to deprecate at a later point. |
|
Per our discussion the dynamic support seems to be equivalent to the account key authentication path. /lgtm |
|
/retest |
2 similar comments
|
/retest |
|
/retest |
|
/cherrypick release-1.35 |
|
@andyzhangx: new pull request created: #3166 DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
What type of PR is this?
/kind feature
What this PR does / why we need it:
Add
mountWithOAuthTokenvolume context parameter to support cross-tenant scenarios where users provide their own OAuth token via a Kubernetes Secret.This enables first-party users who cannot use kubelet identity or workload identity (cross-tenant) to mount Azure File shares using an externally-managed OAuth token.
Changes:
mountWithOAuthTokenoauthtokensetCredentialCacheto support direct token passing toazfilesauthmanager setGetStorageAccountFromSecretto return OAuth token from secretsetCredentialCacheWithOAuthToken— reads OAuth token from K8s Secret and updates credential cachemountWithManagedIdentityandmountWithWorkloadIdentityTokensec=krb5mount)NodeStageVolume: performs initial mount with OAuth token credential cache setupNodePublishVolume: refreshes OAuth token credential cache on every publish (even when already mounted), with SHA256-based skip to avoid unnecessaryazfilesauthmanagercalls when the token is unchangedgetSecretNamespacehelper for consistent namespace resolution (secretNamespace → pvcNamespace → "default")secretNamerequired, empty server returnsInvalidArgumentUsage Example
1. Create Secret with OAuth Token
kubectl create secret generic azure-oauth-token-secret --from-literal=oauthtoken="<oauth-token>"2. Dynamic Provisioning (StorageClass)
3. PV (Static Provisioning)
Security Considerations
Which issue(s) this PR fixes:
Fixes #3099
Does this PR introduce a user-facing change?